Add a vector class to GridKit#418
Conversation
pelesh
requested review from
PhilipFackler,
abdourahmanbarry,
lukelowry and
nkoukpaizan
PhilipFackler
left a comment
PhilipFackler
left a comment
There was a problem hiding this comment.
For an all-in-one solution like this, I would prefer some revisions as explained in my line comments. None of those is a deal-breaker if this is meant only for "advanced" use-cases, but I still think it would be helpful.
All of your suggestions are spot on and should be incorporated before this PR is merged. |
lukelowry
left a comment
lukelowry
left a comment
There was a problem hiding this comment.
I think we need a zombie approach if we do merge this implementation. We should take the best parts of this and graft them onto a more scalable implementation that can isolate the Component in memory (is "closure" the right word here?).
nkoukpaizan
left a comment
nkoukpaizan
left a comment
There was a problem hiding this comment.
A few minor comments.
I'm not convinced the class in GridKit needs to mirror Re::Solve, and I tend to prefer solutions that are driven by the application needs first, but I'm willing to see what happens when we try to use this in GridKit.
shakedregev
left a comment
shakedregev
left a comment
There was a problem hiding this comment.
Address the comment and clarify that this is ready for testing.
| { | ||
| // std::cout << x->getData("cpu")[i] << "\n"; |
There was a problem hiding this comment.
I believe this commented line was used for debugging. We can remove it before merging.
6 participants
Description
Add vector class to GridKit to replace
std::vectorand provide portable data management.Proposed changes
The
LinearAlgebra::Vectorclass was ported from Re::Solve. It has basic initialization and data management functionality. I can be use as a vector, multivector or vector view class.Checklist
-Wall -Wpedantic -Wconversion -Wextra.Further comments
The multivector capability in this class should replace
DenseMatrixobject in GridKit.